Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: provide cyclone base images to build images #1407

Merged
merged 1 commit into from
Apr 13, 2020
Merged

refactor: provide cyclone base images to build images #1407

merged 1 commit into from
Apr 13, 2020

Conversation

zhujian7
Copy link
Collaborator

@zhujian7 zhujian7 commented Apr 8, 2020

What this PR does / why we need it:

Add your description

Which issue(s) this PR is related to (optional, link to 3rd issue(s)):

Fixes ##1390

Reference to #

Special notes for your reviewer:

/cc @supereagle @hyy0322 @chechaoa

Release note:

NONE

@caicloud-bot caicloud-bot added release-note-none Denotes a PR that doesn't merit a release note. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 8, 2020
hyy0322
hyy0322 reviewed Apr 8, 2020
View reviewed changes
build/cicd/cd/Dockerfile.arm64
@@ -1,4 +1,4 @@
FROM arm64v8/alpine:3.8
FROM caicloud/cyclone-base-alpine:v1.0.0-arm64v8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arm64v8 是不是不要放在 tag 里区分，我感觉放在 project 里区分比较好一些

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

打扰了 dockerhub 不支持

@zhujian7
Copy link
Collaborator Author

zhujian7 commented Apr 8, 2020

/hold

@caicloud-bot caicloud-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2020
@zhujian7
Copy link
Collaborator Author

zhujian7 commented Apr 8, 2020

/hold cancel

@caicloud-bot caicloud-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2020
@hyy0322
Copy link
Contributor

hyy0322 commented Apr 8, 2020

LGTM

@hyy0322
Copy link
Contributor

hyy0322 commented Apr 9, 2020

/lgtm

@caicloud-bot caicloud-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2020
@chechaoa
Copy link

chechaoa commented Apr 9, 2020

/lgtm

supereagle
supereagle reviewed Apr 11, 2020
View reviewed changes
Makefile Outdated
@@ -37,11 +38,17 @@ IMAGE_SUFFIX ?= $(strip )

# REGISTRY ?= docker.io/library
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Makefile

# Container registry for base images.
# Container registry for build base images.
BASE_REGISTRY ?= docker.io/library
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not update this?

Makefile
BASE_REGISTRY ?= docker.io/library

# Container registry for cyclone target base images.
CYCLONE_BASE_REGISTRY ?= docker.io/caicloud
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not need this, can reuse the REGISTRY, as target images and base images will been in the same registry
.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CYCLONE_BASE_REGISTRY does not need updating frequently, it's used as base images in the Dockerfiles, and these Dockerfiles Base images will not be updated frequently.

Makefile
BASE_REGISTRY ?= docker.io/library

# Container registry for cyclone target base images.
CYCLONE_BASE_REGISTRY ?= docker.io/caicloud
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not need this, can reuse the REGISTRY, as the target images and base images will been in the same registry.

Makefile
CYCLONE_BASE_REGISTRY ?= docker.io/caicloud

# Version of the cyclone base images.
CYCLONE_BASE_VERSION ?= v1.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prefix CYCLONE_ is not necessary.

Suggested change
CYCLONE_BASE_VERSION ?= v1.0.0
BASE_VERSION ?= v1.0.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to keep consistent with CYCLONE_BASE_REGISTRY.

@caicloud-bot caicloud-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2020
@supereagle
Copy link
Member

/lgtm

@caicloud-bot caicloud-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2020
@caicloud-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chechaoa, hyy0322, supereagle

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@caicloud-bot caicloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2020
@caicloud-bot caicloud-bot merged commit 5bf8183 into caicloud:cps-2.8 Apr 13, 2020
@zhujian7
Copy link
Collaborator Author

/cherrypick cps-2.10

@caicloud-bot caicloud-bot mentioned this pull request Jun 28, 2020
Merged
@caicloud-bot
Copy link
Collaborator

@zhujian7: new pull request created: #1452

In response to this:

/cherrypick cps-2.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyy0322 hyy0322 hyy0322 left review comments

@supereagle supereagle supereagle left review comments

@chechaoa chechaoa Awaiting requested review from chechaoa

Assignees

@supereagle supereagle

@hyy0322 hyy0322

@chechaoa chechaoa

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zhujian7 @hyy0322 @chechaoa @supereagle @caicloud-bot